fix(context): make ContextGraph.add_edge idempotent by deduping on edge_id - #926
Conversation
|
ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing |
PR Summary by QodoMake ContextGraph.add_edge idempotent by deduping edges on edge_id
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
Code Review by Qodo🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)
Great, no issues found!Qodo reviewed your code and found no material issues that require reviewTip of the day💡 Did you know, you can enable the Remediation agent and Qodo fixes findings in a dedicated fix PR |
Adds an Unreleased/Fixed entry for semantica-agi#922/semantica-agi#926 so the ContextGraph edge-dedupe bug and its fix are recorded per Keep a Changelog format.
KaifAhmad1
left a comment
There was a problem hiding this comment.
Verified this end-to-end, not just read the diff — pulled the branch into a worktree and ran it:
_edge_indexdedupe check sits inside the existingself._lock(RLock, so no deadlock with the outer lock inadd_edge), runs before node auto-creation and before the mutation callback fires — a repeatadd_edgeis a true no-op, no phantomADD_EDGEaudit event.- Grepped every write path to
self.edges—_add_internal_edgeis the only append site, and all callers (bulkadd_edges, builders,load_from_file,merge, decision-graph helpers) route through it, so the index can't go stale. Bothload_from_file()andclear()also clear_edge_index. - Ran all three repro scripts from #922 directly against this branch:
edges=1/density=0.5for repeatedadd_edge(was 3/1.5), parallel edges with distinct attributes still produce 3 distinct IDs, and triple re-ingest viabuild_from_entities_and_relationshipsstays atedges=1across all 3 runs (was 1→2→3). - Full
tests/context/test_context.py: 31/31 passed, including the 4 new regression tests. - Checked the interaction with
add_edge's pre-existing SKOS hierarchy cycle check — re-adding an identicalskos:broader/narroweredge doesn't trip a false cycle error.
Clean fix, mirrors the existing node-dedupe pattern, good test coverage. Also resolves the interaction with #852's Markdown export guard as described.
Thanks for the thorough writeup and reproducible repro cases in the issue, @pravit-amp — made this easy to verify. Approving.
edge_id is content-derived and not yet guaranteed unique (semantica-agi#922, fix pending in semantica-agi#926): two identical add_edge() calls produce two edge objects sharing one id. retract_edge()/purge_edge() resolved "the edge" via the first matching object only, so a duplicate was silently left untouched (still live, still active) while the call returned True and recorded a tombstone/retraction claiming it was fully handled. Repeat purge_edge() calls also silently overwrote the tombstone's reason/purged_at on each partial attempt instead of no-op'ing once nothing remained to purge. retract_node()'s cascade had the same root cause from the other direction: it checked the live _retractions dict mid-loop, so the first duplicate's just-written record made the second look already handled and it was skipped outright, left permanently active. retract_edge()/purge_edge() now act on every edge matching the id under a single record; the cascade's dedup check is snapshotted before the loop starts so within-call duplicates are still closed rather than skipped. Adds TestDuplicateEdgeId (5 tests) reproducing all three paths.
* feat(context): add retraction and purge to ContextGraph ContextGraph had 56 public methods and none that removed anything: the only option was clear(), which discards the whole graph. Removing one entity meant exporting to a dict, filtering by hand and rebuilding, losing provenance. Add two operations with deliberately different contracts. retract_node/retract_edge close the entity's validity window. The entity stops being active going forward, but state_at() before the retraction still returns it, so decisions recorded against it remain explainable. This reuses the valid_from/valid_until machinery already present rather than adding a new subsystem. purge_node/purge_edge remove the entity outright, from history as well as from the active view, leaving a tombstone that records that a purge happened and why but never the purged content. Scope is this graph only; copies in AgentMemory or a bound vector store are not reached, so it is one step of an erasure workflow rather than the whole of it. Both record themselves through the existing mutation_callback path. MutationRecord already documented REMOVE_NODE/REMOVE_EDGE in its operation vocabulary, so retraction emits UPDATE_NODE and purge emits REMOVE_NODE with no changes required to change_management. Incident-edge lookup scans self.edges rather than _adjacency, which is keyed by source only and would otherwise leave inbound edges pointing at a removed node. Purge updates edges, edge_type_index and _adjacency together so the indexes cannot drift, and clear() now resets the retraction and tombstone records. * fix(context): address review findings on retraction and purge * fix(context): close every duplicate when retracting/purging by edge_id edge_id is content-derived and not yet guaranteed unique (#922, fix pending in #926): two identical add_edge() calls produce two edge objects sharing one id. retract_edge()/purge_edge() resolved "the edge" via the first matching object only, so a duplicate was silently left untouched (still live, still active) while the call returned True and recorded a tombstone/retraction claiming it was fully handled. Repeat purge_edge() calls also silently overwrote the tombstone's reason/purged_at on each partial attempt instead of no-op'ing once nothing remained to purge. retract_node()'s cascade had the same root cause from the other direction: it checked the live _retractions dict mid-loop, so the first duplicate's just-written record made the second look already handled and it was skipped outright, left permanently active. retract_edge()/purge_edge() now act on every edge matching the id under a single record; the cascade's dedup check is snapshotted before the loop starts so within-call duplicates are still closed rather than skipped. Adds TestDuplicateEdgeId (5 tests) reproducing all three paths. * docs(changelog): document retraction/purge feature Adds an Unreleased/Added entry for #955/#957 covering retract_node, retract_edge, purge_node, purge_edge and the get/list accessors, plus the duplicate-edge_id fix caught and applied during review. --------- Co-authored-by: Pravit Ampapathini <pravitampapathini@Pravits-MacBook-Air-3.local> Co-authored-by: KaifAhmad1 <kaifahmad087@gmail.com>
Fixes #922
ContextGraph._add_internal_edgeappended every edge unconditionally, so adding the same edge twice stored two copies sharing one content-derivededge_id. This inflatedstats()["edge_count"], pusheddensity()past 1.0, and made re-ingest double the edge set on every run.Changes
edge_id -> ContextEdgeindex (_edge_index), mirroring howself.nodesdedupes by node ID._add_internal_edgenow returnsFalsewhen theedge_idalready exists, before touchingedges,edge_type_index, or_adjacency, and before firing the mutation callback (no phantom ADD_EDGE audit events).loadandclear()) also clear the new index.add_edgeis a no-op, parallel edges with distinct attributes are preserved, re-ingest viabuild_from_entities_and_relationshipsstays at one edge, andclear()resets the dedupe index.Notes
Testing
tests/context/suite passes (484 tests).